-
Notifications
You must be signed in to change notification settings - Fork 152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(TimelineStep): move subLabel below timeline #4202
Conversation
Deploying with Cloudflare Pages
|
Size Change: +168 B (0%) Total Size: 460 kB
ℹ️ View Unchanged
|
be7758b
to
e7c9582
Compare
{hasSubLabelMargin && ( | ||
<StyledText> | ||
<Text align="center" size="small"> | ||
{subLabel} | ||
</Text> | ||
</StyledText> | ||
)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will always render a div
with a p
inside even if subLabel
is undefined. That doesn't seem right to me. Since we are at it, we could make it better. Can't we do something like:
Check if it has subLabel:
- If it does, render the text (we don't need the StyledText wrapper with the min-height because the text already has a line-height bigger than that anyway)
- If it does not but
hasSubLabelMargin === true
then we can apply a top margin to the label text to add the spacing.
Something like:
{subLabel && (
<StyledText>
<Text align="center" size="small">
{subLabel}
</Text>
</StyledText>
)}
<StyledText active={active || (last && type === "success")}>
<Text
align="center"
weight="bold"
margin={{ top: !subLabel && hasSubLabelMargin ? "24px" : "0" }}
>
{label}
</Text>
</StyledText>
Note that:
- StyledText here is only needed for the color of the Text. I find this very wrong. Our Text component should just support more colors if we need them. We have the same problem in Itinerary component, where we need to have a
TemporaryText
to support more colors. If we need more colors, let's just support them. I'll bring this topic to discuss on the team in our next meeting. - The hardcoded 24px top margin can be replaced by a token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The StyledText
is out of the scope of this PR, it should be discussed with designers and so on, so it's completely different story ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion, it's better and I made it quite quick so didn't consider it! :)
The discussion of extending Text colors makes sense and let's make it in new year 👍🏻
e7c9582
to
c80b66c
Compare
Related to this and this
Although it was fixed, in case there is no
subLabel
provided to anyTimelineStep
, there is additional spacing that is not desiredStorybook: https://orbit-mainframev-fix-timeline-placement-sublabewl.surge.sh